Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement first pass at simple plugins #261

Merged
merged 8 commits into from
Mar 9, 2023
Merged

Implement first pass at simple plugins #261

merged 8 commits into from
Mar 9, 2023

Conversation

tkkuehn
Copy link
Contributor

@tkkuehn tkkuehn commented Mar 6, 2023 •
edited
Loading

Proposed changes

This is a very simple first crack at resolving #260. It's entirely possible there's some potential complexity I'm missing, so I wanted to open this up for comment before adding to the Sphinx docs. Please feel free to take a look despite the fact that this is in draft mode.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!

  • Changes have been tested to ensure that fix is effective or that a feature works.
  • Changes pass the unit tests
  • I have included necessary documentation or comments (as necessary)
  • Any dependent changes have been merged and published

Notes

All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.

@tkkuehn tkkuehn requested review from pvandyken and kaitj March 7, 2023 14:12
pvandyken
pvandyken reviewed Mar 7, 2023
View reviewed changes
snakebids/app.py Outdated Show resolved Hide resolved
@pvandyken pvandyken linked an issue Mar 7, 2023 that may be closed by this pull request
Basic Plugin Architecture #260
Closed
@pvandyken pvandyken added the enhancement New feature or request label Mar 7, 2023
@tkkuehn tkkuehn marked this pull request as ready for review March 7, 2023 21:30
@tkkuehn tkkuehn requested a review from pvandyken March 7, 2023 21:53
pvandyken
pvandyken requested changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@pvandyken pvandyken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks good, just left some comments on documentation and the test

docs/bids_app/plugins.md Outdated Show resolved Hide resolved
snakebids/app.py Outdated Show resolved Hide resolved
snakebids/app.py Outdated Show resolved Hide resolved
snakebids/app.py Outdated Show resolved Hide resolved
snakebids/tests/test_app.py Outdated Show resolved Hide resolved
@pvandyken
Copy link
Contributor

Also, the docs are building with Sphinx 6.x now, and it looks like it now makes permalinks to docstring attributes. You might be able to reference these with {attr}`attribute` . If you have a chance you can try with the SnakeBidsApp.config reference in the docs, but this isn't super important, so not blocking

@github-actions github-actions bot requested review from akhanf and pvandyken March 8, 2023 15:33
@tkkuehn tkkuehn merged commit ca5560b into khanlab:main Mar 9, 2023
@kaitj kaitj mentioned this pull request May 10, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvandyken pvandyken pvandyken approved these changes

@kaitj kaitj kaitj approved these changes

@akhanf akhanf Awaiting requested review from akhanf

Assignees

@tkkuehn tkkuehn

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Basic Plugin Architecture
3 participants
@tkkuehn @pvandyken @kaitj